Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add llvmdev and llvmlite build GHA workflows #1122

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

swap357
Copy link
Contributor

@swap357 swap357 commented Jan 10, 2025

This PR introduces a new GitHub Actions workflows (llvmdev_build.yml and llvmlite_build.yml) to automate the building and testing of llvmdlv and llvmlite wheels across multiple platforms and Python versions.

Key Features

  • cross-platform builds for:
    • linux (ubuntu-24.04)
    • windows (windows-2019)
    • macos (macos-13)
  • python version matrix support:
    • python 3.10, 3.11, 3.12 and 3.13
  • automated testing of built wheels
  • artifacts retention - 7 days

tested workflows -
llvmlite : https://github.com/swap357/llvmlite/actions/runs/12722849384
llvmdev : https://github.com/swap357/llvmlite/actions/runs/12719219323
llvmdlv_manylinux : https://github.com/swap357/llvmlite/actions/runs/12722220311

* add llvmdev gha workflow

* update build recipe and script

* add llvmlite build workflow

* update artifact naming

* update llvmlite build using conda recipe

* update llvmlite build using conda recipe

* update llvmlite build using conda recipe

* update llvmlite build using conda recipe

* update llvmlite build using conda recipe

* add libllvm15 to recipe meta.yaml

* remove libllvm15 to recipe meta.yaml

* switch to using llvmdev from numba ci channel

* update llvmlite build

* update llvmlite build

* update llvmlite build

* update llvmlite build

* update llvmlite build

* update llvmlite build

* build with setuptools

* remove llvmdev

* add llvmdev

* add win builds

* add win builds

* update win builds

* update win builds

* Add macOS build workflow for multiple Python versions
@swap357 swap357 marked this pull request as ready for review January 10, 2025 06:24
cd buildscripts/manylinux
./docker_run_x64.sh build_llvmdev.sh "${MINICONDA_FILE}"
RECIPE_NAME=./conda-recipes/llvmdev_manylinux
echo "OUTPUT=$(conda build --output $RECIPE_NAME)" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this serving a purpose? The OUTPUT variable doesn't appear to be used in any subsequent step, and the command appears to be erroring:

conda: error: argument COMMAND: invalid choice: 'build' (choose from activate, clean, commands, compare, config, create, deactivate, env, export, info, init, install, list, notices, package, content-trust, doctor, repoquery, remove, uninstall, rename, run, search, update, upgrade)

(from https://github.com/swap357/llvmlite/actions/runs/12704394168/job/35413712445#step:3:27279)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, should’ve been removed since build is currently using script

Comment on lines +106 to +107
echo "LLVM_CONFIG=C:/Miniconda3/envs/build_env/Library/bin/llvm-config" >> $GITHUB_ENV
echo "LLVM_DIR=C:/Miniconda3/envs/build_env/Library/lib/cmake/llvm" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look like unix path separators rather than Windows... It seems to be working (I guess) but is it needed to be in this format? Or is it a slight inconsistency with other parts of this workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these were set using bash, remains of experiments. probably should keep everything consistent and use cmd for windows, will change

Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I've had a look over the workflows and I have a few questions in addition to those on the diff:

  • Is the intention to build manylinux wheels with this? If so, is building on Ubuntu 24.04 appropriate?
  • The artifact retention of one day for llvmdev builds seems a bit short - I could imagine the artifact getting deleted before it can be used / inspected / finished with / whatever, for example if someone started the workflow on a Friday with the hope of continuing whatever task was in progress on the Monday. I imagine artifact storage limits are a consideration (though I'm not familiar with them, I don't know how easy it will be to hit them), but does it seem possible to bump the retention up to 7 days?
  • Presently the llvmlite workflow uses an llvmdev package from the numba channel, rather than the llvmdev workflow. Is the eventual aim to be able to use an artifact from the llvmdev workflow to build llvmlite? (If so that would cut down a lot of hassle for future LLVM feature development in llvmlite - for example the present decoupling has been a huge drag on the compiler-rt work, which still hasn't got over the line yet)
  • Will this be used to build artifacts for upload to PyPI eventually? If so, should sdists be added? Is the intention to add them in the future?
  • The test env for llvmlite is the build env (build_env). Would it be better to use a fresh env for the test, so that there's less chance of something carrying over from the build env that would otherwise be missed in a fresh env?
  • In the past we discussed renaming the manylinux and llvmdev_manylinux folders (I think with @sklam) because they're not just used to build manylinux wheels, it's confusing. I'm not suggesting we want to rename them for this PR (because it might make the review more complicated) but it looks like we ought to rationalise that name soon after this PR is merged.

@swap357
Copy link
Contributor Author

swap357 commented Jan 11, 2025

Thanks for the PR! I've had a look over the workflows and I have a few questions in addition to those on the diff:

  • Is the intention to build manylinux wheels with this? If so, is building on Ubuntu 24.04 appropriate?
  • The ubuntu 24.04 runner is being used only to host the manylinux Docker container. Actual build happens inside the manylinux container. And we can use GitHub Actions' free tier runner.
  • The artifact retention of one day for llvmdev builds seems a bit short - I could imagine the artifact getting deleted before it can be used / inspected / finished with / whatever, for example if someone started the workflow on a Friday with the hope of continuing whatever task was in progress on the Monday. I imagine artifact storage limits are a consideration (though I'm not familiar with them, I don't know how easy it will be to hit them), but does it seem possible to bump the retention up to 7 days?
  • yes, extended all artifacts retention to 7 days now. This can be stretched to 90 days max but it's a tradeoff, would bloat repo storage if workflow is used more frequently for dev. We can adjust this based on team feedback and actual usage patterns.
  • Presently the llvmlite workflow uses an llvmdev package from the numba channel, rather than the llvmdev workflow. Is the eventual aim to be able to use an artifact from the llvmdev workflow to build llvmlite? (If so that would cut down a lot of hassle for future LLVM feature development in llvmlite - for example the present decoupling has been a huge drag on the compiler-rt work, which still hasn't got over the line yet)
  • yes, that's the plan. Ideally we should be able to use llvmdev artifact onto llvmlite and then llvmlite artifact onto numba. I'm working on it but wanted to have this PR to have something to start with.
  • Will this be used to build artifacts for upload to PyPI eventually? If so, should sdists be added? Is the intention to add them in the future?
  • could be used, but this will bring security concerns to discuss and address.
  • The test env for llvmlite is the build env (build_env). Would it be better to use a fresh env for the test, so that there's less chance of something carrying over from the build env that would otherwise be missed in a fresh env?
  • addressing this.
  • In the past we discussed renaming the manylinux and llvmdev_manylinux folders (I think with @sklam) because they're not just used to build manylinux wheels, it's confusing. I'm not suggesting we want to rename them for this PR (because it might make the review more complicated) but it looks like we ought to rationalise that name soon after this PR is merged.
  • I've separated manylinux build (using ./conda-recipes/llvmdev_manylinux with manylinux docker) and regular builds (using ./conda-recipes/llvmdev) workflows to make this easier. should I remove windows content from manylinux recipe ?

@gmarkall
Copy link
Member

@swap357 Thanks for the responses / updates - I'll take a look again on Monday. In the meantime, could I ask you to not force-push rewritten history please? It makes it very hard / impossible to track progress of the review and changes in the GitHub UI.

@swap357
Copy link
Contributor Author

swap357 commented Jan 13, 2025

@swap357 Thanks for the responses / updates - I'll take a look again on Monday. In the meantime, could I ask you to not force-push rewritten history please? It makes it very hard / impossible to track progress of the review and changes in the GitHub UI.

accidentally pushed experimental commits onto main, when I wanted to checkin squashed commits for test env change, will be mindful onwards

@gmarkall gmarkall requested a review from esc January 14, 2025 17:09
@gmarkall gmarkall added this to the v0.45.0 milestone Jan 14, 2025
@swap357
Copy link
Contributor Author

swap357 commented Jan 15, 2025

  • separated llvmlite manylinux and regular builds.
  • llvmdev artifact can now be used on llvmlite workflow. added optional workflow id field which will allow importing generated artifacts from llvmdev workflows.
  • if no workflow id is provided, it uses numba/label/ci channel to get llvmdev with conda.

image

@esc
Copy link
Member

esc commented Jan 15, 2025

@swap357 Thanks for the responses / updates - I'll take a look again on Monday. In the meantime, could I ask you to not force-push rewritten history please? It makes it very hard / impossible to track progress of the review and changes in the GitHub UI.

accidentally pushed experimental commits onto main, when I wanted to checkin squashed commits for test env change, will be mindful onwards

Would recommend to use feature branches: https://www.atlassian.com/git/tutorials/comparing-workflows/feature-branch-workflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants